Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

QMAPS-2088 Use ?q= intention bbox as initial map position #1066

Merged
merged 13 commits into from
Apr 27, 2021

Conversation

bbecquet
Copy link
Contributor

@bbecquet bbecquet commented Apr 16, 2021

Description

Add a new Express middleware to interpret incoming root urls containing a ?q= param. If it resolves to a multi-place intention or a geocoding result, redirect to either a /places or a /place url, allowing to use the bbox or POI center as the initial map position (as now supported by #1064 and #1055).

Why

Prevents the map initializing on a different start position before "jumping" to the bbox when the data is asynchronously fetched by the client app.

@bbecquet bbecquet changed the title Qmaps 2088 Use ?q= intention bbox as initial map position QMAPS-2088 Use ?q= intention bbox as initial map position Apr 16, 2021
@bbecquet bbecquet force-pushed the QMAPS-2088-initial-map-position-q branch from 21d3474 to b32fca4 Compare April 16, 2021 10:27
@bbecquet bbecquet added the WIP label Apr 19, 2021
@bbecquet bbecquet force-pushed the QMAPS-2088-initial-map-position-q branch 2 times, most recently from 6fae6fd to 41c56f6 Compare April 19, 2021 16:40
bin/middlewares/fullText_query.js Outdated Show resolved Hide resolved
bin/middlewares/fullText_query.js Outdated Show resolved Hide resolved
Comment on lines 67 to 71
getRedirectUrl(req.query.q, res)
.then(redirectUrl => {
if (redirectUrl) {
res.redirect(307, redirectUrl);
} else {
next();
}
})
.catch(error => {
req.logger.error({ err: error });
next();
});
Copy link
Contributor

@amatissart amatissart Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible regression with this redirect mechanism is that the raw query is not preserved in the search bar.
The expected behavior in this case is unclear (also discussed in #1051).
Do you think it would be relevant to pass the raw query as an extra parameter in the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the new way things will work with the search bar, I believe it would be overwritten in any case by the POI name… I don't agree with this behavior but the design team seems to be quite positive with it.

I can pass it in any case to make it available to the front to decide what to do with it.

@bbecquet bbecquet removed the WIP label Apr 20, 2021
@bbecquet bbecquet added WIP and removed WIP labels Apr 20, 2021
@bbecquet bbecquet force-pushed the QMAPS-2088-initial-map-position-q branch from 3bb8c00 to 5a587d7 Compare April 20, 2021 17:30
@bbecquet bbecquet requested a review from G-Ray April 23, 2021 13:18
Copy link
Contributor

@amatissart amatissart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About telemetry events

This PR impacts 2 telemetry events:

  • app_start: direct_search field used to be true when q was present as a direct search query
  • suggest_selection: from_url_query field used to be true for the selection triggered by a direct search.

I would suggest to simply remove these fields that introduce unnecessary complexity, and use the existing ?client to distinguish between the different sources of queries.
The mechanism used to trigger a search based on the URL parameter could actually be removed completely on the client side.

Then, the new middleware "fullText_query.js" could forward the client parameter from the original URL (if present), and set client=direct-search by default in the redirect URL.

@bbecquet What do you think ? And should this cleanup be done in a separate PR?

bin/middlewares/fullText_query.js Outdated Show resolved Hide resolved
@bbecquet bbecquet force-pushed the QMAPS-2088-initial-map-position-q branch from b0504c7 to 5c4363f Compare April 27, 2021 08:30
@bbecquet
Copy link
Contributor Author

@amatissart Thanks for the encoding remark.
About the telemetry/simplification, I'll do it in a separate PR as agreed IRL :)

@bbecquet bbecquet merged commit c1cef55 into Qwant:master Apr 27, 2021
@bbecquet bbecquet deleted the QMAPS-2088-initial-map-position-q branch April 27, 2021 09:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants